Skip to content

Conversation

james-elicx
Copy link
Contributor

@james-elicx james-elicx commented Sep 1, 2025

per opennextjs/opennextjs-cloudflare#862 (review)

Using this PR, the CLI will treat absolute paths as absolute, instead of always treating them as relative. Relative paths should not start with a /, i.e. you should use ./config-path.ts instead of /config-path.ts.

The behavior of the CLI is unchanged after this PR, that is:

open-next build --config-path /relative/path

will continue to treat /relative/path as relative to the baseDir == process.cwd().

Copy link

changeset-bot bot commented Sep 1, 2025

🦋 Changeset detected

Latest commit: 859bd8f

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages
Name Type
@opennextjs/aws Patch
app-pages-router Patch
app-router Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link

pkg-pr-new bot commented Sep 1, 2025

Open in StackBlitz

pnpm add https://pkg.pr.new/@opennextjs/aws@972

commit: 859bd8f

@james-elicx james-elicx marked this pull request as ready for review September 1, 2025 08:05
Copy link
Contributor

@vicb vicb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

Copy link
Contributor

@sommeeeer sommeeeer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, we should probably update the docs to address this change once merged. I did test this with an absolute path:

# This did work :)
node /home/user/open-next/dist/index.js build --config-path ~/hi/open-next.config.ts

@vicb
Copy link
Contributor

vicb commented Sep 1, 2025

LGTM, we should probably update the docs to address this change once merged. I did test this with an absolute path:

# This did work :)
node /home/user/open-next/dist/index.js build --config-path ~/hi/open-next.config.ts

A couple comments:

I didn't realize earlier that aws supports --config-path.

Would the current PR be a change in behavior, i.e. build --config-path /path/to would have been considered a relative path before but now requires build --config-path ./path/to.

If that is the case this is a change in behavior. I believe we can call this a "fix" rather than a breaking change but it would be nice to add details in the changeset.

Second comment is that we can use --config-path in cloudflare too, WDYT @james-elicx ?
I don't have a strong preference on this one. On one hand this would make both the CLI consistent but on the other hand cloudflare already uses --config to point to a wrangler config.
So I'm fine either way but probably something we can discuss before merging the cloudflare PR.

@james-elicx james-elicx changed the title feat: support absolute paths when compiling opennext configs fix: support absolute paths for OpenNext configs Sep 1, 2025
@james-elicx
Copy link
Contributor Author

LGTM, we should probably update the docs to address this change once merged. I did test this with an absolute path:

# This did work :)
node /home/user/open-next/dist/index.js build --config-path ~/hi/open-next.config.ts

A couple comments:

I didn't realize earlier that aws supports --config-path.

Would the current PR be a change in behavior, i.e. build --config-path /path/to would have been considered a relative path before but now requires build --config-path ./path/to.

If that is the case this is a change in behavior. I believe we can call this a "fix" rather than a breaking change but it would be nice to add details in the changeset.

Good point. I've updated the changeset. Alternatively, we could remove the support for passing in absolute paths in the AWS CLI and leave it until they do a future breaking change?

Second comment is that we can use --config-path in cloudflare too, WDYT @james-elicx ? I don't have a strong preference on this one. On one hand this would make both the CLI consistent but on the other hand cloudflare already uses --config to point to a wrangler config. So I'm fine either way but probably something we can discuss before merging the cloudflare PR.

That would be a breaking change because we use the config path flag for the Wrangler config as part of allowing users to pass any wrangler flags to the cli.

@vicb
Copy link
Contributor

vicb commented Sep 1, 2025

Good point. I've updated the changeset. Alternatively, we could remove the support for passing in absolute paths in the AWS CLI and leave it until they do a future breaking change?

@conico974 and others, WDYT?

That would be a breaking change because we use the config path flag for the Wrangler config as part of allowing users to pass any wrangler flags to the cli.

I think it's -c / --config as I wrote earlier though?
i.e. it would not be a breaking change but --config vs --config-path could be confusing (the first one being the wrangler config and the second one the ON config)

@james-elicx
Copy link
Contributor Author

That would be a breaking change because we use the config path flag for the Wrangler config as part of allowing users to pass any wrangler flags to the cli.

I think it's -c / --config as I wrote earlier though? i.e. it would not be a breaking change but --config vs --config-path could be confusing (the first one being the wrangler config and the second one the ON config)

Ah yes, I see, Wrangler's is config but we also gave configPath as the flag name in cloudflare, with config and c as aliases 🤦‍♂️. So that would be a breaking change still in cloudflare.

@sommeeeer
Copy link
Contributor

Would the current PR be a change in behavior, i.e. build --config-path /path/to would have been considered a relative path before but now requires build --config-path ./path/to.

Ah, good catch! I confirmed this is the case.

@vicb
Copy link
Contributor

vicb commented Sep 1, 2025

Wrangler's is config but we also gave configPath as the flag name in cloudflare, with config and c as aliases 🤦‍♂️. So that would be a breaking change still in cloudflare.

@james-elicx What about:

  • rename the arg to config with a single c alias (instead of configPath)
  • add a "new" arg configPath

The new behavior would be:

  • error early when both flags are set
  • use either config or configPath when only one of them is set
  • print a deprecation warning when configPath is used
  • remove the deprecated configPath in the next major release

Copy link
Contributor

@vicb vicb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@james-elicx I have updated the PR description to describe the breaking change in this PR:

open-next build --config-path /relative/to/baseDir

will not treat /relative/to/baseDir as an absolute path (you would have to use ./relative/to/baseDir)

I think this is still correct?

If it is, we probably do not want that breaking outside of a major aws release

@james-elicx
Copy link
Contributor Author

Alternatively, we could remove the support for passing in absolute paths in the AWS CLI and leave it until they do a future breaking change?

Yes, that's why I asked if we should remove that part of the change until AWS does a major release. I have no problem removing that if people prefer not to do it as part of this

@vicb
Copy link
Contributor

vicb commented Sep 3, 2025

Yes, that's why I asked if we should remove that part of the change until AWS does a major release. I have no problem removing that if people prefer not to do it as part of this

We should not introduce a breaking change in aws, confirmed offline with @conico974.

For Cloudflare we have to support both absolute (/...) and relative (./...) paths - that how wrangler --c(onfig) works and we don't want a different behavior for the Open Next config flag - that is consistency with the Cloudflare CLI is more important than consistency across aws and clouflare.

Ideally this PR can be updated to allow cf CLI support for both absolute and relative paths while not changing the behavior of aws CLI (always relative).

@james-elicx james-elicx changed the title fix: support absolute paths for OpenNext configs fix: support passing absolute paths for compiling OpenNext configs Sep 3, 2025
@james-elicx james-elicx force-pushed the james/absolute-config-path branch from 323935d to ff9988e Compare September 3, 2025 13:05
@james-elicx
Copy link
Contributor Author

Thanks for confirming. I've removed that part of the change.

@james-elicx james-elicx requested a review from vicb September 3, 2025 13:22
@@ -0,0 +1,7 @@
---
"@opennextjs/aws": minor
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we usually treat internal API changes as patch changes.

I'm fine with either but maybe @conico974 has a stronger opinion?

@vicb
Copy link
Contributor

vicb commented Sep 4, 2025

@james-elicx I have added a proposal fixup to this PR, might help speed up merging this - or not :)

The idea is that:

  • The CLI behavior is not changed in open-next build --config-path /relative/path, /relative/path is still relative to the baseDir
  • The internal compileOpenNextConfig() takes openNextConfigPath instead of openNextConfigPath and baseDir. openNextConfigPath is actually more akin to what's we usually call a path, that is absolute when starting with a /.

This change is a patch/refactor because only an internal API is not backward compatible.

Minor changes also in this proposal:

  • more JS doc
  • make compileOpenNextConfigEdge return the path to the config file to be consistent with the Node conterpart.

What do you think about the proposal?

Copy link
Contributor

@conico974 conico974 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry guys i took so long to respond here. I was convinced I did 😄
LGTM, I definitely didn't want to introduce a breaking change here

@james-elicx
Copy link
Contributor Author

@james-elicx I have added a proposal fixup to this PR, might help speed up merging this - or not :)

The idea is that:

  • The CLI behavior is not changed in open-next build --config-path /relative/path, /relative/path is still relative to the baseDir
  • The internal compileOpenNextConfig() takes openNextConfigPath instead of openNextConfigPath and baseDir. openNextConfigPath is actually more akin to what's we usually call a path, that is absolute when starting with a /.

This change is a patch/refactor because only an internal API is not backward compatible.

Minor changes also in this proposal:

  • more JS doc
  • make compileOpenNextConfigEdge return the path to the config file to be consistent with the Node conterpart.

What do you think about the proposal?

Looks like they were just nitpicks about variable names and jsdoc, so looks good to me.

@vicb vicb changed the title fix: support passing absolute paths for compiling OpenNext configs refactor: openNextConfigPath now take a path to the config file Sep 5, 2025
@vicb vicb changed the title refactor: openNextConfigPath now take a path to the config file refactor: compileOpenNextConfig now take a path to the config file Sep 5, 2025
@vicb
Copy link
Contributor

vicb commented Sep 5, 2025

Looks like they were just nitpicks about variable names and jsdoc, so looks good to me.

Right!

Thanks for your work on this PR!

@vicb vicb merged commit 3994aca into opennextjs:main Sep 5, 2025
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants